fix: replace hardcoded npmx.dev & docs.npmx.dev with env-specific URLs #1402
Conversation
Extract a composable that returns URLs for this app itself (and docs, which lives in this codebase), so that URLs are always internally consistent within an environment, e.g. in dev links go to dev.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe pull request centralises URL configuration by introducing a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
| : undefined | ||
|
|
||
| export const getSiteUrl = (isDevelopment: boolean) => { | ||
| if (isDevelopment) return 'http://localhost:3000' |
There was a problem hiding this comment.
wondering if we should or should not hardcode this? It is possible to pass flags with the dev command to change the port/host. Could we get that from somewhere else?
There was a problem hiding this comment.
unfortunately only 127.0.0.1 works for the atproto oauth callback
but yes, you can get the actual host and port in module space
There was a problem hiding this comment.
as I have a feature PR stacked on this one and PRs get stale fast here 😅 and this PR leaves things no worse than before, how would you feel about following up on this later?
| return { | ||
| siteUrl, | ||
| // TODO(serhalp): Handle preview environment. The docs site is a separate deployment, so we'd | ||
| // need to infer its preview URL from the site preview URL somehow...? |
There was a problem hiding this comment.
https://vercel.com/docs/deployments/generated-urls#generated-from-git
the URL's are deterministic, so can we just construct them during the build?
There was a problem hiding this comment.
as I have a feature PR stacked on this one and PRs get stale fast here 😅 and this PR leaves things no worse than before, how would you feel about following up on this later?
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
maybe you could explain a bit more about why?
I'm not sure I see a benefit here - it's straightforward enough in local development to open the docs in a new tab, and canonical urls or json-ld aren't really relevant either for previews or local development.
in the mean time, if it's blocking another work-in-progress, maybe you could rebase that branch to drop these commits and we can revisit it later?
... or maybe someone else has an opinion?
| export function useAppUrls() { | ||
| const { env, siteUrl } = useAppConfig() | ||
| return { | ||
| siteUrl, | ||
| // TODO(serhalp): Handle preview environment. The docs site is a separate deployment, so we'd | ||
| // need to infer its preview URL from the site preview URL somehow...? | ||
| docsUrl: env === 'dev' ? 'http://localhost:3001' : NPMX_DOCS_SITE_PROD, | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure how useful this is - we can't get the preview url of a docs site, and we don't have a hard-coded port for the docs running locally (and this ends up being runtime code we don't really need in the site)
| // Canonical URL for this org page | ||
| const canonicalUrl = computed(() => `https://npmx.dev/@${orgName.value}`) | ||
| const { siteUrl } = useAppUrls() | ||
| const canonicalUrl = computed(() => `${siteUrl}/@${orgName.value}`) |
There was a problem hiding this comment.
this ends up meaning that preview deployments get their own canonical url, and I would try to avoid that. just like with og images, you want to point to the canonical https://npmx.dev so that search engines don't end up indexing preview deployments accidentally
(I know we have a header instructing them not to, but this seems safe to hard-code. if we ever need to switch https://npmx.dev, we could easily do it throughout the codebase without much effort.)
In #1405 I'm adding a link to one of our doc pages from within npmx. When you're in local dev, this link would otherwise go to prod, unlike every other internal link (I think of it as an "internal" link...). I assume over time we'll be adding more and more deep links to npmx docs within npmx, and more and more contributors will waste time being confused by this (not seeing local changes reflected, 404 navigating to a new page, etc.). Maybe I've just been bitten too many damn times by this during my career... In any case, to keep things clean I'll rebase my feat PR to remove these commits and hardcode the prod docs domain for now. 👍🏼 |
4 participants
Extract a composable that returns URLs for this app itself (and docs, which lives in this codebase), so that URLs are always internally consistent within an environment, e.g. in dev links go to dev.